normalize-yaml: Enhance to collapse spacing#9980
Conversation
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM, this isn't strictly necessary because HTML collapses extra spaces, so it's just for consistency in source?
There was a problem hiding this comment.
QQ how come we need the capture group? My expectation is that we could just grab the spaces and period?
| node.value = node.value.replace(/(\w)\. {2,}/g, '$1. '); | |
| node.value = node.value.replace(/\. {2,}/g, '. '); |
There was a problem hiding this comment.
QQ how come we need the capture group? My expectation is that we could just grab the spaces and period?
In my mind I was thinking the capture group helped assure that it was actually a sentence of English (French, etc.) text vs. some garbled symbols or some other non-"sentence" content like a numbered list, but (a) that's probably unlikely and (b) we may want the spacing collapse for those scenarios anyways?
There was a problem hiding this comment.
Yeah I agree we'd want to collapse anyways, and we could make the rule smarter if we run into issues in the future as needed?
There was a problem hiding this comment.
Maybe it's safe to even go so far as to collapse all multiple spaces everywhere, regardless of whether it occurs after a period? The resulting diff from existing issues seems reasonable.
diff --git a/app/javascript/packages/normalize-yaml/visitors/collapse-sentence-spacing.js b/app/javascript/packages/normalize-yaml/visitors/collapse-sentence-spacing.js
index 1ac577e9af..e4f5eaf8b1 100644
--- a/app/javascript/packages/normalize-yaml/visitors/collapse-sentence-spacing.js
+++ b/app/javascript/packages/normalize-yaml/visitors/collapse-sentence-spacing.js
@@ -3,3 +3,3 @@ export default /** @type {import('yaml').visitor} */ ({
if (typeof node.value === 'string') {
- node.value = node.value.replace(/(\w)\. {2,}/g, '$1. ');
+ node.value = node.value.replace(/ {2,}/g, ' ');
}
diff --git a/config/locales/titles/fr.yml b/config/locales/titles/fr.yml
index ed8e21acab..c79bac9bf3 100644
--- a/config/locales/titles/fr.yml
+++ b/config/locales/titles/fr.yml
@@ -47,3 +47,3 @@ fr:
suggest_second_mfa: Vous avez ajouté votre première méthode d’authentification !
- Ajoutez-en une deuxième en guise de sauvegarde.
+ Ajoutez-en une deuxième en guise de sauvegarde.
no_auth_option: Aucun message de connexion trouvé
diff --git a/config/locales/user_mailer/en.yml b/config/locales/user_mailer/en.yml
index 0a68b4b203..aaa84453c1 100644
--- a/config/locales/user_mailer/en.yml
+++ b/config/locales/user_mailer/en.yml
@@ -285,3 +285,3 @@ en:
gpo_letter_header: Your letter is on the way
- header: To finish resetting your password, please click the link below or copy
+ header: To finish resetting your password, please click the link below or copy
and paste the entire link into your browser.
diff --git a/config/locales/zxcvbn/fr.yml b/config/locales/zxcvbn/fr.yml
index cea1f76730..01666fb39f 100644
--- a/config/locales/zxcvbn/fr.yml
+++ b/config/locales/zxcvbn/fr.yml
@@ -28,3 +28,3 @@ fr:
Les répétitions comme « abcabcabc » sont à peine
- plus difficiles à deviner que « abc »
+ plus difficiles à deviner que « abc »
reversed_words_arent_much_harder_to_guess: Les mots inversés ne sont pas très difficiles à deviner
That's a good point to highlight. Technically it's not always so simple, but that's largely true, yes. There could be cases where these strings are used in SMS or email messages, which may not necessarily work the same, but also probably an edge case. But similar to line length (Prettier) formatting, I think it's fair to say that source consistency is a big factor, yes. |
changelog: User-Facing Improvements, Content, Improve consistency of spacing between content sentences
See: #9980 (comment) Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
aduth
force-pushed
the
aduth-normalize-yaml-collapse-spaces
branch
from
1375ad8 to
0391204
Compare
aduth
changed the title
3 participants
🛠 Summary of changes
Adds new behavior to the YAML normalization package to collapse spacing
between sentencesin YAML content.Related discussion: https://gsa-tts.slack.com/archives/CEUQ9FXNJ/p1706111249614589
📜 Testing Plan
Verify that
make lint_yamlis produces a successful exit code.make lint_yamlecho $?0Verify that reverting included normalizations and running normalization applies the changes to collapse spaces
git checkout main -- config/localesmake normalize_yamlgit add config/localesgit statusVerify that included specs pass:
yarn mocha app/javascript/packages/normalize-yaml/index.spec.js